Skip to content

fix(test): make the reconcile-lease tests deterministic - #181

Merged
zaridan merged 2 commits into
mainfrom
fix/test-gmail-watch-lease-flake
Aug 2, 2026
Merged

fix(test): make the reconcile-lease tests deterministic#181
zaridan merged 2 commits into
mainfrom
fix/test-gmail-watch-lease-flake

Conversation

@zaridan

@zaridan zaridan commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

🟢 SAFE TO MERGE

Test-only change. No new decisions. All gates green on f9ab2fe, including the full Quality run (typecheck, lint, test, coverage). Codex (adversarial, in place of CodeRabbit — rate limited, no walkthrough produced): 6 findings — 4 real and fixed, 1 wrong, 1 valid but out of scope (raised below for the maintainer).


src/store/gmail-watch-state.test.ts fails on roughly half of all runs of the file alone — observed 4 of 6 on clean main, before any change. A gate that fails half the time is not a gate; it masks real regressions. This makes it deterministic.

src/store/gmail-watch-state.ts is untouched.

Root cause

The lease token is claimed_untilnow() + leaseMs — and PGlite's now() advances only in whole milliseconds (verified: its ::text rendering never carries sub-millisecond digits). Two claims of the same leaseMs landing in the same millisecond therefore render the identical token:

AssertionError: expected '2026-08-02 11:48:36.747-08'
             not to be '2026-08-02 11:48:36.747-08'

Both failing assertions were expect(second).not.toBe(first) — in "claiming succeeds again once the previous lease has expired, returning a NEW token" and "a stale holder cannot clobber a live successor's lease".

The expireReconcileLease helper rewound claimed_until with UPDATE ... SET claimed_until = now() - interval '1 second'. That fakes the expiry without the elapsed time the expiry implies, so the successor's claim ran in the same millisecond as its predecessor's and the two tokens came back equal.

In production a successor can only claim once the prior lease has actually expired — i.e. at least leaseMs later — so its claimed_until is necessarily later and the tokens necessarily differ, whatever the clock's precision. The SQL shortcut was the only thing removing that guarantee.

Not a sub-millisecond boundary in the token comparison, and not a flaw in the token-as-text design (store module doc, "Why the token is text, not a Date") — that reasoning holds unchanged.

Fix

Expire the lease the way production does: advance the clock. PGlite reads the JS system clock for now() (verified), so the lease describe block pins a frozen, fixed clock and fakes Date only:

vi.useFakeTimers({ toFake: ['Date'] })   // setTimeout stays real, so nothing PGlite relies on stalls
vi.setSystemTime(FROZEN_NOW)             // constant instant — no wall-clock dependence left

and the helper jumps the clock to leaseMs + 1 ms past the claim. No retries, no widened tolerances, no sleeps. Both tests assert exactly what they asserted before.

Verification

  1. Reproduced first on unmodified main — 4 of 6 runs failed.
  2. 12/12 consecutive green runs of the file alone (re-run after the review fixes).
  3. Not vacuous — reverting the store's AND claimed_until = $2::timestamptz release guard still fails the stale-holder test, so the safety property is still under test.
  4. Full suite: 1340 passed, 0 failed. tsc: 0 errors in the changed file. Biome: clean.

Review adjudication

CodeRabbit was rate limited (Review limit reached, no walkthrough, no inline comments, no review object) so an adversarial Codex pass was run in its place, prompted against the specific invariants at risk. Its 6 findings:

# Finding Verdict
1 Assert explicitly that PGlite's now() follows the faked clock, or a future backend change silently breaks the time travel Wrong. The test fails closed: if the clock stopped coupling, the tokens would collide again and both assertions would fail loudly. A separate assertion buys nothing.
2 Fake timers inherit the wall clock, leaving real-clock dependence Real — fixed (f9ab2fe). Clock pinned to a constant instant.
3 The token is a timestamp, not a true ownership proof; a µs collision plus a retried release could clobber a successor Valid observation, out of scope — see below.
4 +1s skips the strict claimed_until < now() boundary Real — fixed (f9ab2fe). Now +1 ms, the smallest step PGlite resolves.
5 Helper calls vi.setSystemTime at file scope, outside the block that installs fake timers Real — fixed (f9ab2fe). Moved inside the block.
6 The doc comment overstates what is modelled Real — fixed (f9ab2fe). It models the expiry, not elapsed time.

Finding 3 — flagged for the maintainer, not fixed here

The reviewer is right that the lease token is a timestamp, not an identifier, so it is not a true ownership proof. Its concrete failure scenario is not reachable in this codebase today: it requires two claims colliding within one microsecond of real Postgres's now() and a retried release — and gmail-reconcile.ts:629 releases exactly once inside a finally, catching and swallowing errors without retry.

Making the token a real ownership proof would mean a dedicated random lease_token column — a schema migration and a product change, well outside a test-only de-flake. Raising it rather than acting on it.

Scope notes

  • Only this file had the pattern. src/mail/ingest.test.ts and src/providers/adapters/postgres-queue/index.race.test.ts use the same SQL lease-rewind but never compare two tokens, so they are not exposed.
  • Unrelated, pre-existing: 14 test files fail to load locally because imapflow and nodemailer are declared in package.json but not installed. Zero test failures result. Not touched here.

🤖 Generated with Claude Code

`src/store/gmail-watch-state.test.ts` failed on roughly half of all runs
of the file alone (observed 4/6 on clean main), on either or both of the
two assertions that compare one lease token against another:

    AssertionError: expected '2026-08-02 11:48:36.747-08'
                 not to be '2026-08-02 11:48:36.747-08'

## Root cause

The lease token IS `claimed_until` — `now() + leaseMs` — and PGlite's
`now()` advances only in whole MILLISECONDS (verified: its `::text`
rendering never carries sub-millisecond digits). Two claims of the same
`leaseMs` landing in the same millisecond therefore render the identical
token.

The `expireReconcileLease` helper rewound `claimed_until` in SQL, which
faked the expiry WITHOUT the elapsed time that expiry implies — so the
successor's claim ran in the same millisecond as its predecessor's and
the two tokens came back equal.

In production the successor can only claim once the prior lease has
actually expired, i.e. at least `leaseMs` later, so its `claimed_until`
is necessarily later and the tokens necessarily differ regardless of
clock precision. The SQL shortcut was the only thing removing that
guarantee.

## Fix

Expire the lease the way production does: advance the clock. PGlite
reads the JS system clock for `now()` (verified), so the lease `describe`
block fakes `Date` only — `vi.useFakeTimers({ toFake: ['Date'] })`,
leaving `setTimeout` real so nothing PGlite relies on stalls — and the
helper moves the clock past `leaseMs`.

No retries, no widened tolerances, no sleeps; the tests assert exactly
what they asserted before.

## Verification

- Reproduced first on unmodified main: 4/6 runs failed.
- 12/12 consecutive green runs of the file alone after the fix.
- Not vacuous: reverting the store's `AND claimed_until = $2::timestamptz`
  release guard still fails the stale-holder test.
- Full suite 1340 passed / 0 failed; `tsc` clean for this file; Biome clean.

Test-only change — `src/store/gmail-watch-state.ts` is untouched.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@vercel

vercel Bot commented Aug 2, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
helpthread Ready Ready Preview Aug 2, 2026 8:26pm
helpthread-inbox Ready Ready Preview Aug 2, 2026 8:26pm

Request Review

@coderabbitai

coderabbitai Bot commented Aug 2, 2026

Copy link
Copy Markdown

Warning

Review limit reached

@zaridan, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 19 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 310adcdf-7f7c-4da8-8f17-1dbbf442dfd0

📥 Commits

Reviewing files that changed from the base of the PR and between 78f1069 and f9ab2fe.

📒 Files selected for processing (1)
  • src/store/gmail-watch-state.test.ts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Adversarial review follow-ups (Codex substitute, CodeRabbit rate limited):

- Pin the faked clock to a constant instant rather than inheriting the
  wall clock at hook time. The assertions were all relative so this was
  not a flake source, but the point of the change is to remove real-clock
  dependence from the block, and inheriting `Date.now()` left some.
- Advance by `leaseMs + 1` ms rather than `+ 1s`. One millisecond is the
  smallest step PGlite's `now()` can resolve, so the claim guard's strict
  `claimed_until < now()` boundary stays under test instead of being
  cleared by a wide margin.
- Move `expireReconcileLease` inside the describe block that installs the
  fake timers — it calls `vi.setSystemTime` unconditionally, so at file
  scope a future caller outside that block would hit a non-obvious throw.
- Tighten the helper's doc: it models the expiry, not the passage of
  time — the clock stays frozen at its new instant until moved again.

Re-verified: 12/12 consecutive green runs; the stale-holder test still
fails when the store's `AND claimed_until = $2::timestamptz` release
guard is reverted, so it remains non-vacuous.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@zaridan
zaridan merged commit a9e3a35 into main Aug 2, 2026
8 checks passed
@zaridan
zaridan deleted the fix/test-gmail-watch-lease-flake branch August 2, 2026 21:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant